-
Notifications
You must be signed in to change notification settings - Fork 1.9k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Fix missing host header in http check #15337
Conversation
Disclaimer: I have only learned go ~30 minutes ago. So check this properly.One architectural issue with this solution is that by using a host header on our side the model doesn't match cleanly with the Go This means that to support this use case, on setting headers we need to create an exception for the I'd argue that a cleaner solution would be to adopt the same model as |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for this PR @SamMousa! I think you're probably right that this isn't quite the right place to handle this, and I've left a comment for an approach you might want to try.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
After the rewrite and finding out how http.Header
is implemented, it does not feel like a very strong contract to me.
The http.Header
type is internally defined as type Header map[string][]string
and does not give us any stronger guarantees; all it does it add some helper functions.
headers: func() map[string][]string { | ||
h := makeHeaders(encoding, agent, [2]string{"Test-Abc", "hello"}) | ||
h["hoST"] = []string{"heLLO"} | ||
return h | ||
}(), | ||
}, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We still test sending in weird data although we don't expect our code to ever send us this data since everything is normalized at the edge.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hi @SamMousa! Thanks for digging into this further. I think we're getting closer, but something about the change in nomad/structs/services.go
tickled that paranoid part of my brain and I checked the encoding round trip. We'll need to back out that part unfortunately.
Once that's done I think we're in good shape here. You can run make cl
and that'll walk you through creating a changelog entry file in .changelog/15337.txt
that you can add to this PR.
nomad/structs/services.go
Outdated
InitialStatus string // Initial status of the check | ||
TLSSkipVerify bool // Skip TLS verification when Protocol=https | ||
Method string // HTTP Method to use (GET by default) | ||
Header http.Header // HTTP Headers for Consul to set when making HTTP checks |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You'd rightfully think that encoding shouldn't care about a type alias. But between Go and optimized MessagePack encoding we can't have nice things. 😀 Changing this value breaks the MessagePack encoding we use to serialize these structs to raft. If you run make generate-structs
and then:
func Test_EncodeServiceCheck(t *testing.T) {
check := &structs.ServiceCheck{
Name: uuid.Generate(),
Type: "http",
Header: map[string][]string{"Host": {"example.com"}},
}
data := []byte{}
buf := bytes.NewBuffer(data)
encoder := codec.NewEncoder(buf, structs.MsgpackHandle)
err := encoder.Encode(check)
must.NoError(t, err)
err = os.WriteFile("encoded.msgpack", buf.Bytes(), 0o700)
must.NoError(t, err)
}
Then make this change and run make generate-structs
again the following test will fail!
func Test_DecodeServiceCheck(t *testing.T) {
data, err := os.ReadFile("encoded.msgpack")
must.NoError(t, err)
buf := bytes.NewBuffer(data)
decoder := codec.NewDecoder(buf, structs.MsgpackHandle)
var check *structs.ServiceCheck
err = decoder.Decode(check)
must.NoError(t, err)
must.NotNil(t, check)
must.Eq(t, "example.com", check.Header.Get("Host"))
}
$ go test -v -count=1 ./nomad -run Test_DecodeServiceCheck
# github.com/hashicorp/nomad/nomad.test
ld: warning: -no_pie is deprecated when targeting new OS versions
=== RUN Test_DecodeServiceCheck
assert.go:24:
service_registration_endpoint_test.go:1547: expected nil error
↪ error: msgpack decode error [pos 1]: cannot decode into value of kind: ptr, type: *structs.ServiceCheck, <nil>
--- FAIL: Test_DecodeServiceCheck (0.00s)
FAIL
FAIL github.com/hashicorp/nomad/nomad 0.525s
FAIL
So unfortunately for backwards compatibility with existing raft entries, we'll need to leave this field untouched and only fix it up in client/serviceregistration/checks/client.go
), | ||
}, | ||
{ | ||
name: "host header", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nit: this is the second test case named "host header"
; making them unique helps quickly tracking down failures
Done! Couldn't this script get the metadata directly from the PR? It asks for the PR number and the type is derivable from the tag you add to the PR. ( |
} | ||
} | ||
|
||
request.Host = request.Header.Get("Host") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Note for future readers of this PR: this is safe without checking for it being empty because:
// For client requests, Host optionally overrides the Host
// header to send. If empty, the Request.Write method uses
// the value of URL.Host. Host may contain an international
// domain name.
Oh that's a neat idea. I think we'd just need to make sure we have labels that correspond to each of the states. Some PRs actually end up having multiple changelog entries as well (ex. breaking change + bug fix), and folks would still need to write the changelog itself (which often shouldn't be 1:1 with the commit message). |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM!
Thanks @SamMousa! This will ship in the next regular version of Nomad (likely 1.4.4) |
I'm going to lock this pull request because it has been closed for 120 days ⏳. This helps our maintainers find and focus on the active contributions. |
Fixes #15308